fix(call): detect dead signaling WebSocket connections via ping interval - #6543
fix(call): detect dead signaling WebSocket connections via ping interval#6543tareko wants to merge 1 commit into
Conversation
The signaling WebSocket was created from the shared OkHttpClient without a ping interval. In OkHttp 4.x this means no protocol-level pings are sent and the WebSocket read timeout is infinite, so a half-open connection (e.g. after switching from WiFi to cellular without a TCP reset) is never detected: onFailure never fires, the existing reconnect logic never runs, and the call goes silently mute/deaf while participants still appear present. Derive a dedicated client for the signaling WebSocket that pings every 10 seconds. OkHttp now fails the socket when a pong is not received in time, which triggers the existing onFailure -> restartWebSocket path. Regular HTTP calls keep using the unchanged shared client. Assisted-by: opencode:ox-alpha Signed-off-by: Tarek Loubani <tarek@tarek.org>
There was a problem hiding this comment.
Pull request overview
Adds a dedicated signaling WebSocket client with a 10-second ping interval to detect dead connections and trigger reconnection.
Changes:
- Configures signaling-specific WebSocket pings.
- Preserves the shared HTTP client.
- Adds client configuration tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Summary |
|---|---|
app/src/test/java/com/nextcloud/talk/webrtc/WebSocketInstanceSignalingClientTest.kt |
Tests currently compare Long values against Int literals on lines 32 and 49; use 10_000L. |
app/src/main/java/com/nextcloud/talk/webrtc/WebSocketInstance.kt |
Uses the dedicated ping-enabled signaling client. |
Suppressed comments (2)
app/src/test/java/com/nextcloud/talk/webrtc/WebSocketInstanceSignalingClientTest.kt:49
pingIntervalMillisis aLong, while0is anInt. This therefore selects JUnit's object overload and compares anIntegerto aLong, causing the unchanged-base-client assertion to fail every time. Use0Lhere.
0,
app/src/test/java/com/nextcloud/talk/webrtc/WebSocketInstanceSignalingClientTest.kt:55
- This new test file ends immediately after the closing brace. Please add the repository-required single empty trailing line so the file follows the required file-format convention.
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // hardcoded on purpose: fails if the ping interval in WebSocketInstance changes or is removed | ||
| assertEquals( | ||
| "signaling WebSocket client must send pings to detect half-open connections", | ||
| 10_000, |
There was a problem hiding this comment.
Thanks for the look, but I am pretty sure this is incorrect. The types are the other way around. OkHttp declares pingIntervalMillis as Int (public final int pingIntervalMillis() in okhttp3.OkHttpClient, see https://github.com/square/okhttp/blob/parent-4.12.0/okhttp/src/main/kotlin/okhttp3/OkHttpClient.kt). With both operands Int, Kotlin widens to the primitive assertEquals(long, long) overload — there is no boxed Integer/Long comparison.
An earlier iteration of this test actually used 10_000L and failed precisely because of the boxed Long vs Integer mismatch the comment describes — the Int literal is the deliberate fix, and the test passes: ./gradlew testGenericDebugUnitTest --tests "com.nextcloud.talk.webrtc.WebSocketInstanceSignalingClientTest" (same for line 49, 0 vs Int).
The failing CI jobs on this PR are unrelated infrastructure issues (lint runner OOM, a canceled qa runner, and the integration server container failing to come up).
fix(call): detect dead signaling WebSocket connections via ping interval
Description
The signaling WebSocket is currently created from the shared
OkHttpClient, which never sets a ping interval. In OkHttp 4.x this means no protocol-level pings are sent and the WebSocket read timeout is effectively infinite. When the underlying network dies without a TCP reset — the normal case when switching from WiFi to cellular, or when a router/NAT drops an idle mapping — the connection becomes half-open:onFailurenever fires (TCP retransmission timeouts can take 15+ minutes)isConnectedstaystrue, so the existing reconnect logic never runsThe result is a "zombie" call: participants still see each other, but the call is mute/deaf. This is the root cause of the failure mode reported in #2368 and #1760 (analysis also posted on #2368).
This PR derives a dedicated client for signaling WebSocket connections with a 10 second ping interval. OkHttp now fails the socket when a pong is not received in time, which triggers the existing
onFailure→restartWebSocket()path. The shared client used for regular HTTP calls is unchanged (ping intervals have no effect on non-WebSocket calls, but the dedicated instance keeps the change scoped).Steps to reproduce / How to test
Without this PR: the call silently stops working — no reconnect, participants appear present but nothing is transmitted, for 15+ minutes.
With this PR: within ~10–20 seconds the dead socket fails,
onFailuretriggers, and the existing reconnect flow runs (reconnect + hello/resume).Also test that normal call behavior is unaffected: join/leave calls, chat while connected, both WiFi and cellular, with and without HPB.
/backport to stable-24.0may be worthwhile given No automatic reconnect on instable connection #2368 affects older versionsFixes part of #2368
Note: This change was developed with AI assistance (opencode / Kimi K3 and GLM-5.3).